fix(tui,mcp): show MCP servers that failed to start in /mcp, bound to their server and with credentials redacted - #835

The failed-state wiring and secret redaction look good, but the new failure-reason rendering needs terminal sanitization before merge.

BuildMCPViewState passes redaction.ErrorMessage(err, ...) into MCPServerView.Error, and mcpManagerServerLines inserts that value directly into the rendered lines. Redaction removes credentials but does not remove ANSI/OSC sequences, other control characters, or embedded newlines. I reproduced this with an MCP error containing connection refused\x1b[2J\n› forged · enabled; the resulting server line retained both the escape sequence and newline unchanged. A server-controlled handshake error can therefore manipulate the terminal or forge extra /mcp rows.

Please normalize the displayed reason to safe single-line terminal text: strip ANSI/OSC and control characters, flatten CR/LF, apply a reasonable length cap, and add a regression covering escape and newline injection.

Everything else in the change looks correct, and the focused tests and CI are green.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/tui/mcp_view.go`:
- Around line 192-238: Bound MCPServerView.Error before processing in the
sanitizer around the visible rune conversion and strings.Builder accumulation.
Verify whether Runtime.Skipped() already imposes a strict size limit; if not,
limit the raw input before converting to []rune and stop accumulating once the
maxMCPReasonLen display budget is reached, while preserving ANSI stripping,
whitespace normalization, and truncation behavior.

Verdict: Approve

Verified empirically on the branch (checked out, built).

What I checked

• Gut-the-fix: forcing the failure branch off (mcp_state.go:71) so a skipped server renders as "enabled" turns the TUI MCP tests red. The tests exercise the behavior.
• Precedence is right and tested: a server that is both disabled and recorded-failed shows as "disabled", not "failed" (mcp_state.go:66-69), and this is asserted directly (mcp_failed_state_test.go:45 — "disabled to win over a recorded failure"). Correct — a server you turned off was never expected to connect.
• Reason is redacted before display (redaction.ErrorMessage, mcp_state.go:73) — invariant 6, so a path/credential in a startup error can't leak into the panel. Empty-error fallback ("server did not start") is handled too.
• Skipped set reaches the panel via MCPSkipped: mcpRuntime.Skipped(); companion to #822 which did the same for zero mcp check.
• Clean scope — every file is the MCP panel or its plumbing.

Worth a quick confirm (non-blocking)

• On reconnect (zero mcp enable after a failure), the panel is rebuilt from a fresh Skipped set, so it should flip back to enabled — worth a sanity check that the runtime clears the entry on a successful re-register.

Good fix.

I found issues that need to be addressed before this is ready.

Merge and review state

• Review decision: GitHub still reports CHANGES_REQUESTED (anandh8x’s review on be580766 is not dismissed). Terminal sanitization and raw-input bounding from that review look addressed on the current head, but the merge gate still needs a fresh approval on 6521c367. gnanam1990 approved on 6521c367; CodeRabbit approved on the same head.
• Mergeability: MERGEABLE, no conflict markers in the PR diff. mergeStateStatus is BLOCKED pending required reviews.
• Checks: All CI / CodeQL / Zero Review / CodeRabbit checks passed on head.

Prior review alignment

gnanam1990’s approval is valid for what they checked: skipped servers map to failed, disabled wins over recorded failure, reasons are redacted at build (redaction.ErrorMessage), MCPSkipped reaches the TUI, and the focused tests go red when the failure branch is gutted. Those checks exercise buildMCPServerViews, renderMCPView, and m.mcpText() — not the bare /mcp manager overlay entry point. Their non-blocking note about enable clearing Skipped on reconnect is separate from this finding; live reconnect from the TUI is out of scope per the author, and mcpSkipped remains a startup snapshot.

That approval does not negate the remaining gap below: state and redaction work on the transcript/renderMCPView path, but the overlay users get from bare /mcp still omits the reason line.

Findings

• [P2] Bare /mcp shows failed in the manager overlay but not the failure reason
  internal/tui/model.go (commandMCP → openMCPManager), internal/tui/mcp_manager.go (mcpManagerOverlay, mcpManagerServerMeta, mcpManagerSelectionDetail), internal/tui/mcp_view.go (mcpManagerServerLines, renderMCPView)
  Empty /mcp has long routed to openMCPManager() — this PR did not change that. What it did change is meaningful: buildMCPServerViews now marks skipped servers as failed, so the overlay meta and detail pane correctly say failed instead of the pre-PR enabled with missing tools. The recorded reason, however, is rendered only in mcpManagerServerLines inside renderMCPView(). The overlay never reads server.Error or calls sanitizeTerminalReason, so a user who types /mcp after a startup warning sees the right state but not the “why” issue #825 and this PR’s description target. The renderMCPView path does show the sanitized reason — on /mcp list and other transcript subcommands, and in transcript output appended after manager actions such as check or list — but the primary overlay surface is still incomplete relative to the stated fix. TestModelMCPPanelReportsStartupFailures exercises m.mcpText() / renderMCPView(), not the bare /mcp entry point. Please surface the sanitized reason in the manager overlay (list meta, selection detail, or both), reusing the same sanitizeTerminalReason path mcpManagerServerLines already uses.

I found issues that need to be addressed before this is ready.

Findings

• [P1] Redact the configured server credentials before retaining its error
  internal/tui/mcp_state.go:73
ErrorMessage is called with empty options, even though remote MCP clients send every value in MCPServerConfig.Headers. A server can echo an arbitrary configured value (for example X-Workspace-Credential: <value>) in its failed startup response; that value is not covered by the generic redaction patterns, is saved in MCPServerView.Error, and is newly rendered in both /mcp surfaces and the session transcript. Pass the configured secret values into redaction (and cover an echoed custom header); this also avoids relying on the syntactic Authorization: matcher, which terminal control bytes can evade before the later sanitizer removes them.

LGTM

Verdict

Approve — and this withdraws my CHANGES_REQUESTED from 072e0366, which is what has been holding this branch alongside jatmn's approval.

Reviewed at 6cc1ec40, base 99721c76.

Withdrawing my previous block

My 2026-09-12 review said explicitly "I found no new code-level blocker" and requested changes only because the branch was 29 commits behind main. That is done: 6cc1ec40 is 0 behind origin/main, so the reason no longer exists. Changes-requested survives a push where an approval does not, so it needed a new submission rather than an edit — hence this review.

What I verified on this head

• Merged tree: git merge origin/main is already-up-to-date; go build ./..., go vet ./... and the full go test ./... are clean on macOS.
• Gut-the-fix on four load-bearing behaviours — each reverted in turn, internal/tui rerun, all four caught:
  1. a recorded failure rendering as failed — revert and the panel shows a server that never started as enabled
  2. the trimmed-identity lookup (strings.TrimSpace(rawName)) — revert to the raw map key and " docs " misses its failure entry, which is the exact defect the comment at mcp_state.go:85-97 describes
  3. disabled winning over a recorded failure
  4. redactMCPFailureReason on the displayed reason — replacing it with a plain err.Error() is caught, so invariant #6 is held by a test and not just by the call site

That last one is the one I most wanted to see fail, since a startup error is exactly where a path or bearer leaks into a panel.

Follow-up from my earlier pass, now closed

I had asked (non-blocking) whether a successful re-register clears the entry so the panel flips back to enabled. The observation now carries its credential fingerprint (SkippedCredentials, mcpCredentialFingerprint) and buildMCPServerViews compares against it, so a stale failure is not rendered against refreshed material. That answers it.

Not covered

macOS only. I did not exercise a real failing MCP server end-to-end, nor the Windows paths; the evidence above is the merged tree plus targeted mutation of the display logic.

The failure reasons come from the server and pass the redaction helper before rendering (with dedicated tests for authorization-header echo, oversized secrets, and path credentials) plus truncation bounds, and disabled wins over failed in the display. Scope note: this bundles credential fingerprints, an OAuth store, and identity work well beyond a fix(tui) title, though it arrives with about 34 test files.

I rechecked this PR on head 27218dc (full diff, focused tests, blind search). One code item remains before I’d approve; the rest of what I raised on 42a95b9 (redaction pipeline, arg boundaries, manager ConfigKey, optional completion, collision/enable paths, etc.) is addressed on current head or is explicitly out of scope for #825.

Merge readiness

• Branch is not behind origin/main; required CI on 27218dc is green.
• My CHANGES_REQUESTED on 42a95b9 is stale relative to this head—treat it as superseded by this note once the finding below is fixed (or tell me if you want any prior thread item re-opened with a specific repro on 27218dc).

🟡 P2 — Use each row’s ConfigKey in transcript MCP action hints

📍 Where: internal/tui/mcp_view.go (mcpServerActionLine, consumed from mcpManagerServerLines in the /mcp transcript text).

💥 What fails: The panel lists suggested CLI commands with the canonical runtime name, not the configuration map key:

actions := []string{"zero mcp check " + name} // name = strings.TrimSpace(server.Name)

For a server stored under a padded key such as " docs ", the row displays as docs but zero mcp check (and siblings) look up cfg.Servers[serverName] by exact key. The manager overlay already dispatches check/disable/enable/remove with ConfigKey; copying the transcript hint runs the wrong command or reports “not configured” while the panel still shows a failed server.

🔎 Root cause: This PR split display identity (Name, trimmed runtime) from action identity (ConfigKey, exact map key) and wired ConfigKey through the manager, but the transcript action line still builds commands from Name only—the last in-diff operator surface missing from the same identity contract you already applied in mcp_manager.go.

📜 Stated contract:

ConfigKey is the EXACT key in the configuration map, untrimmed. Actions address the config by that key, so it is the only identity that can select one row when a padded alias renders under the same canonical name.

🏷️ Attribution: Incomplete fix on this PR at 27218dc. Misleading hints for padded keys existed at merge-base too; what’s new is the documented ConfigKey rule and manager compliance—transcript hints need to match in the same pass.

📌 In this PR:

• internal/tui/mcp_view.go — mcpServerActionLine / mcpManagerServerLines transcript actions (must use ConfigKey)
• internal/tui/mcp_manager.go — already correct (do not regress)

🔒 Unchanged on main: CLI key lookup; stderr / mcp check redaction (still generic redaction.ErrorMessage as before this PR’s panel work).

🔧 Required correction: Build every zero mcp … hint in mcpServerActionLine from server.ConfigKey (shell-quote when the key contains whitespace or shell metacharacters). Add a regression test with a padded key that the action line includes the exact map key, not only the trimmed display name.

🛠️ Author fix: Fix transcript action hints and the test in one pass; leave manager dispatch, CLI, registration, stderr, and mcp check redaction alone unless you’re deliberately expanding scope.

🚫 Out of scope for this round: Panel-grade redaction on stderr / mcp check / ACP; optional MCP “enabled” while background registration runs; view cache keyed on late-failure count only; in-session re-register after config edits (per your PR note).

I found four issues that need to be addressed before this is ready. The latest transcript action-hint fix is correct, including the author's distinction between exact configuration keys and OAuth's normalized names. I also verified the explanations about disabled aliases, the overlap-versus-cut tests, and the deliberately excluded reconnect/stderr behavior.

Reviewed head 2c36c67ac8be29ff13b59b18f580ac656b9cb6e0 against merge-base and live main 4247a07aba0f89c3c2597314a33edaede56be70c.

Merge readiness

The branch is mergeable and 0 commits behind main; no rebase or release-metadata conflict was found. All 12 executed checks succeeded; CodeRabbit was skipped. GitHub reports BLOCKED / CHANGES_REQUESTED, so fresh review approval is needed after the fixes. The closed #822 diagnostic issue is complementary, and merged #1113 supplies the OAuth fields involved below; neither supersedes #835.

Findings

🟠 P1 — Include supported runtime credentials in the new failure-redaction input

📍 Where: internal/mcp/oauth_store.go:286-294 (SecretValues) and internal/tui/mcp_state.go:915-921 (environment collection).

💥 What fails: Three real startup paths disclose an opaque credential in both the transcript panel text and manager detail:

• With auth: oauth and no configured OAuth block, a stored dynamically registered ClientSecret is submitted on refresh. A token endpoint's error_description echo is displayed verbatim.
• A credential in the discovered/stored TokenEndpoint is likewise absent from the new redaction input. A real failing token POST that echoes its workspace query value exposes it.
• A stdio child can inherit COMPANY_TOKEN from the launching environment, as the documented secret-manager setup permits. When the child reports that value while failing initialization, the new reason displays it even though Target contains no credential.

I reproduced the last case with a real child through the existing execution adapter's supported degraded path; its environment scrubber leaves this unlisted variable available. All three corresponding display probes pass at the merge base, where startup error text is not rendered.

🔎 Root cause: The new safety input is reconstructed from configured fields and only part of stored OAuth material:

values = append(values, stored.AccessToken, stored.RefreshToken)
// The configured-environment collector separately visits only raw.Env.

It misses accessible credentials that the supported startup paths actually use. The token-context fingerprints also receive the incomplete stored set.

📜 Stated contract:

“Redact success and error paths (including stderr).” — AGENTS.md

“ClientSecret is sensitive.” — the existing StoredToken contract

“A secret manager that injects the env var your MCP server reads on its own (the command and args then run inside that environment).” — docs/EXTENDING.md

🏷️ Attribution: PR-introduced disclosure. Stored discovery fields and inherited environment already exist on base/main, but the newly added failure-reason path exposes them. This is not a request to repair the older stderr policy.

📌 In this PR:

🔒 Unchanged on main: Login, refresh, environment merging/scrubbing, and token persistence already implement these startup sources. Public client IDs, scopes, expiry, token types and ordinary environment metadata are not credentials.

🔧 Required correction: Extend the PR's new safety-input collection to cover the stored client secret, credential-bearing stored token-endpoint components using the existing URL rules, and inherited credential-named stdio variables such as COMPANY_TOKEN. Keep capture/current context consistent for mutable stored material. Preserve read-only enumeration, existing bounds, ordinary metadata and current configured-value protection. Verify real refresh/child failures through both displays, including relevant credential-change suppression.

🛠️ Author fix: Close the missing-input rule across every row above in one pass, including source comments and regression fixtures. Do not fix only the OAuth example or only the environment example; do not require a new observation architecture or plaintext credential store.

🚫 Out of scope: OAuth protocol changes, sandbox environment policy, introspecting secrets created inside arbitrary wrapper scripts, unrelated logging and reconnect behavior.

🟠 P1 — Preserve query credential provenance across raw and decoded spellings

📍 Where: internal/tui/mcp_state.go:1103-1134 (mcpURLSecretValues).

💥 What fails: Real HTTP registration failures expose configured URL credentials in both displays:

• ?api%5Fkey=%73crt followed by a value-only echo of %73crt: Target recognizes the decoded sensitive key and masks it, but Error prints the recoverable raw credential spelling.
• ?workspace=opaque%2Dworkspace%2D9f3c2b7&filter=a;b followed by a decoded-value echo: the unrelated semicolon error causes the valid decoded workspace candidate to be discarded. An unrelated invalid escape has the same effect. A normal valid-query control is protected.

🔎 Root cause: The raw pass classifies an undecoded key, while the decoded pass is discarded wholesale on any query parse error. Raw and decoded representations therefore lose either their credential classification or their membership in the candidate set.

if isSensitiveMCPDisplayKey(rawKey) { /* ... */ }
query, err := url.ParseQuery(parsed.RawQuery)
if err != nil {
    return known, values
}

📜 Stated contract:

“Redact success and error paths (including stderr).” — AGENTS.md

The existing TestShortValuesUnderSensitiveKeysAreRedacted and TestPercentEncodedEndpointCredentialsAreRedacted assert the specific supported contracts: known credentials bypass the readability floor, and configured escaped/decoded URL values remain protected.

🏷️ Attribution: PR-introduced. The collector and rendered failure reason are new; identical real transport/display probes do not expose these values at base/live main.

📌 In this PR:

🔒 Unchanged on main: URL admission and generic redaction. The accepted URL loader does not reject these query strings; this finding does not require changing that policy.

🔧 Required correction: Classify each raw query value using its decoded key, retaining both supported value spellings with that classification. Preserve independently decodable pairs when another pair fails. Use the corrected shared helper for every listed caller, with regression controls for ordinary short metadata and all demonstrated combinations.

🛠️ Author fix: Fix the query extraction rule and all listed caller coverage together. Do not patch one parameter name or one URL source; do not rewrite core URL validation or generic redaction.

🚫 Out of scope: New encoding schemes, broad configuration rejection rules and unrelated CLI output.

🟠 P1 — Mark interrupted HTTP error details as incomplete

📍 Where: internal/mcp/network_client.go:643-646 (httpStatusError).

💥 What fails: An HTTP or SSE server returns 500 with Content-Length: 2048, sends only the first 24 bytes of a configured 32-byte opaque credential, then closes. The real client retains those bytes, but ErrorDetailTruncated is false. Both new failure displays show Qw7ZmPr4aBcD9eFgH2jKlM6n, 24 of the 32 secret bytes. Complete-body controls remain protected.

🔎 Root cause: The new incompleteness signal recognizes only the size cutoff and discards the body-read failure:

body, _ := io.ReadAll(io.LimitReader(response.Body, maxHTTPErrorDetail+1))
truncated := len(body) > maxHTTPErrorDetail

A short partial body cannot match the complete configured credential, and neither this false marker nor the local 20 KiB bound activates the existing tail repair.

📜 Stated contract:

“Redact success and error paths (including stderr).” — AGENTS.md

The existing TestCredentialPrefixCutByTheHTTPReaderIsNotDisplayed protects credential prefixes cut at the producer; this is an observable interrupted-read variant of that same boundary.

🏷️ Attribution: PR-activated disclosure. Base/main already discard the read error, but do not render this startup body in the MCP views. The PR adds both the reason consumers and the incomplete producer marker. Identical HTTP/SSE display probes pass at base.

📌 In this PR:

• httpStatusError: shared by HTTP POST, SSE stream GET and SSE POST.
• ErrorDetailTruncated / redactMCPFailureReason: existing new consumer correctly repairs marked partial text.
• Both reason renderers: affected through shared state.
• New producer/truncation tests: size-cut and complete-body cases covered; read-error case absent.

🔒 Unchanged on main: Network transport architecture and old stderr/check behavior.

🔧 Required correction: Treat a failed body read as incomplete, or withhold its partial detail while retaining useful status context. Preserve byte bounds and complete-response behavior. If reusing the flag, keep size slicing dependent on actual body length so a short failed read cannot panic. Add interrupted-response regressions through real HTTP/SSE registration and both consumers.

🛠️ Author fix: Close the shared producer incompleteness rule for every listed route in one pass. Reuse the existing marker/tail handling; do not replace the networking framework or expand this into arbitrary partial-string recognition.

🚫 Out of scope: Detecting prefixes deliberately authored as complete error messages or changing unrelated error-output policy.

🟡 P2 — Isolate the new startup-wiring test from user token stores

📍 Where: internal/cli/app_mcp_skipped_test.go:31-68.

💥 What fails: The test leaves newMCPTokenStore at its production default. Running this exact new test with simulated user ZERO_MCP_OAUTH_TOKENS_PATH / ZERO_OAUTH_TOKENS_PATH values migrated the seeded legacy token, renamed its original file and created the unified store. On Windows, the helper sets only APPDATA, while both OAuth paths fall back through HOME/USERPROFILE; an inherited keyring backend also bypasses a temporary config path.

The reproduction used only review-owned simulated user files.

🔎 Root cause: Redirecting the config root does not isolate the credential-store constructor or its migration/backend selection.

📜 Stated contract:

“Tests must not read or write the developer's real config, cache, or state directories.”

“Redirect every root the code under test resolves, not just config.” — AGENTS.md

🏷️ Attribution: PR-introduced test defect. This test file and constructor invocation path are absent at base/live main. The old shared helper's limitations do not justify adding another uncontained caller.

📌 In this PR:

• New startup test: production token constructor and inherited backend/path settings remain active.
• New direct token-store helpers in internal/mcp and internal/tui: already use explicit temporary paths and Env: map[string]string{}.
• New CLI identity/check tests: non-OAuth stdio paths do not invoke this token constructor.

🔒 Unchanged on main: Credential-store and legacy-migration implementations; unrelated older tests.

🔧 Required correction: Supply an explicitly temporary file-backed token store with an isolated environment in this new test, or equivalent complete local isolation that also prevents legacy migration and host-keyring access. The existing dependency seam and store options already support this.

🛠️ Author fix: Contain every credential-store resolution reached by the new startup test in one pass, and verify seeded user paths remain untouched. Do not redesign the store or repair unrelated main tests.

🚫 Out of scope: Shared storage architecture and repository-wide test cleanup.

Needs maintainer decision

A child can interpret --api-key -Q... as a value, but the current collector intentionally treats a following dash-prefixed item as another flag; its negative test explicitly preserves --api-key --verbose. A real-child probe confirms the separated spelling can expose an opaque value, while equals/packed spellings redact it. There is no established child-independent grammar resolving that ambiguity. I am not requiring parser expansion or removal of the existing negative control in this PR; support for that separated form needs an explicit product choice.

Reviewed contract coverage

Malformed-input review covered empty/trailing/next flags, packed and equals boundaries, base64 padding, positional URLs, escaped query/fragment data, invalid URL fallback, independently invalid query pairs, wrong config shapes/types, missing transport requirements, invalid names/hosts/schemes, nil stores/errors, oversized values and UTF-8 cuts. The findings require local safety corrections, not new admission policy.

Validation

• go build ./... and go vet ./internal/tui ./internal/mcp ./internal/redaction ./internal/cli passed.
• Affected-package tests passed for MCP, redaction and CLI. The TUI package's only failure was TestRelativeAgeFormatsLastActivity (2025-05-31 versus 2025-06-01); the identical failure reproduces at base.
• Focused MCP/credential tests with -race passed across TUI, MCP and CLI.
• Review-owned overlay probes exercised real HTTP/SSE/stdio paths, both displays, immutable-base comparisons and simulated user-store migration. The reviewed checkout is unchanged.

I rechecked this PR on head 56c7d55013cd86db5fdb5d89e7c03b745e85c4f1 (full diff, primary review, controller-run blind passes, GitHub/CI evidence). The startup-failure visibility work matches #825, and the redaction/identity fixes from prior review rounds are present on this head.

Merge readiness

• Branch freshness: fix/825-mcp-panel-shows-failures matches live origin/main at 2e10a4b60b5ef8ebbf18bde8aea6dbfc65c7b43b (0 commits behind). Merge-base for the PR diff is 4247a07aba0f89c3c2597314a33edaede56be70c.
• CI: All 12 executed checks on the latest GitHub run are green (Unit Tests & Coverage, Race Detector, smoke matrix, Zero Review, etc.). CodeRabbit skipped.
• Local validation: Review relied on CI plus static inspection of the changed packages (go was not available in the review environment).

Needs maintainer decision

Separated --api-key -<opaque> argv spelling: A stdio child can treat a dash-prefixed next token as another flag; the collector deliberately does not redact that ambiguous shape (see PR “Known limit” and existing negative control around --api-key --verbose). Equals/packed spellings are covered. Whether to expand parsing for the separated form needs an explicit product decision; I am not requiring it in this PR.

Findings

🟢 Thanks for the contribution. I do not see any actionable issues from my review on head 56c7d550.

Prior CHANGES_REQUESTED items on older heads (stored OAuth material in the redaction input, per-pair query classification, HTTP body read incompleteness, startup test token-store isolation, transcript ConfigKey action hints) are addressed in the current tree. Out-of-scope items called out in the PR and earlier reviews remain out of scope here (in-session reconnect after enable, panel-grade redaction on stderr / bare zero mcp check output, separated dash-prefixed credential argv without a child grammar).